Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Btabaska/74 pally ci api setup #2017

Closed
wants to merge 22 commits into from

Conversation

btabaska
Copy link
Collaborator

Summary

Fixes #{ISSUE}

Time to review: x mins

Changes proposed

What was added, updated, or removed in this PR.

Context for reviewers

Testing instructions, background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers. Explain how the changes were verified.

Additional information

Screenshots, GIF demos, code examples or output to help show the changes working as expected.

acouch and others added 22 commits May 14, 2024 15:39
## Summary
Fixes HHS#1757 

## Changes proposed
- Add e2e tests for search page
- Add e2e test (with FE and API) to its own CI job
(`ci-frontend-ci.yml`)
    - invokes shell script to wait until API is loaded
## Summary
Fixes #37 

## Changes proposed
- add some of the relevant tests from bug bash
## Summary
Fixes HHS#1957

## Changes proposed
- Update sortby labels and ordering
### Time to review: __1 mins__

## Changes proposed
Needed to upgrade dependencies for the API for grype issue:
https://github.com/navapbc/simpler-grants-gov/actions/runs/9180615894/job/25245519194?pr=47

## Additional information
As usual, just ran `poetry update`
## Summary
Fixes #9

### Time to review: __10 mins__

## Changes proposed
Setup a search index to run locally via Docker

Updated makefile to automatically initialize the index + added a script
to wait for the index to start up before proceeding.

Setup a very basic client for connecting to the search index (will be
expanded more in subsequent PRs)

Basic test / test utils to verify it is working (also will be expanded)

## Context for reviewers
This is the first step in getting the search index working locally. This
actually gets it running, and the client works, we just aren't doing
anything meaningful with it yet besides tests.

## Additional information

This doesn't yet create an index that we can use, except in the test.
However, if you want to test out a search index, you can go to
http://localhost:5601/app/dev_tools#/console (after running `make init`)
to run some queries against the (one node) cluster.
https://opensearch.org/docs/latest/getting-started/communicate/#sending-requests-in-dev-tools
provides some examples of how to create + use indexes that you can
follow.
… the index (#44)

## Summary
Fixes #12

### Time to review: __5 mins__

## Changes proposed
Made a new set of v1 endpoints that are basically copy-pastes of the
v0.1 opportunity endpoints

## Context for reviewers
Some changes I want to make to the schemas wouldn't make sense without
the search index (eg. adding the filter counts to the response). As we
have no idea what the actual launch of the v0.1 endpoint is going to
look like, I don't want to mess with any of that code or try to make a
weird hacky approach that needs to account for both the DB
implementation and the search index one.

Also, I think we've heard that with the launch of the search index,
we'll be "officially" launched, so might as well call in v1 at the same
time.

Other than adjusting the names of a few schemas in v0.1, I left that
implementation alone and just copied the boilerplate that I'll fill out
in subsequent tickets.

## Additional information
The endpoint appears locally:
![Screenshot 2024-05-20 at 12 18 32
PM](https://github.com/navapbc/simpler-grants-gov/assets/46358556/86231ec1-417a-41c6-ad88-3d06bb6214e5)

---------

Co-authored-by: nava-platform-bot <[email protected]>
## Summary
Fixes #10

### Time to review: __10 mins__

## Changes proposed
Setup a script to populate the search index by loading opportunities
from the DB, jsonify'ing them, loading them into a new index, and then
aliasing that index.

Several utilities were created for simplifying working with the
OpenSearch client (a wrapper for setting up configuration / patterns)

## Context for reviewers
Iterating over the opportunities and doing something with them is a
common pattern in several of our scripts, so nothing is really different
there.

The meaningful implementation is how we handle creating and aliasing the
index. In OpenSearch you can give any index an alias (including putting
multiple indexes behind the same alias). The approach is pretty simple:
* Create an index
* Load opportunities into the index
* Atomically swap the index backing the `opportunity-index-alias`
* Delete the old index if they exist

This approach means that our search endpoint just needs to query the
alias, and we can keep making new indexes and swapping them out behind
the scenes. Because we could remake the index every few minutes, if we
ever need to re-configure things like the number of shards, or any other
index-creation configuration, we just update that in this script and
wait for it to run again.

## Additional information
I ran this locally after loading `83250` records, and it took about 61s.

You can run this locally yourself by doing:
```sh
make init
make db-seed-local
poetry run flask load-search-data load-opportunity-data
```

If you'd like to see the data, you can test it out on
http://localhost:5601/app/dev_tools#/console - here is an example query
that filters by the word `research` across a few fields and filters to
just forecasted/posted.

```json
GET opportunity-index-alias/_search
{
  "size": 25,
  "from": 0,
  "query": {
    "bool": {
      "must": [
        {
          "simple_query_string": {
            "query": "research",
            "default_operator": "AND", 
            "fields": ["agency.keyword^16", "opportunity_title^2", "opportunity_number^12", "summary.summary_description", "opportunity_assistance_listings.assistance_listing_number^10", "opportunity_assistance_listings.program_title^4"]
          }
        }
      ],
      "filter": [
        {
          "terms": {
            "opportunity_status": [
              "forecasted",
              "posted"
            ]
          }
        }
      ]
    }
  }
}

```
## Summary
Fixes #6 

### Time to review: __60 mins__

## Changes proposed

### Move pages from page to app router:

1. Move all pages to
[`[locale]`](https://next-intl-docs.vercel.app/docs/getting-started/app-router/with-i18n-routing#getting-started)
folder
2. Add
[`generateMetata()`](https://nextjs.org/docs/app/api-reference/functions/generate-metadata#generatemetadata-function)
function and [next-intl
`getTranslations()`](https://next-intl-docs.vercel.app/docs/environments/metadata-route-handlers#metadata-api)
implementation
* @rylew1 commented we could remove this from each page. To do that we
could use [prop
arguments](https://nextjs.org/docs/app/api-reference/functions/generate-metadata#with-segment-props)
and update the based on the param. There is also more we can do with the
metadata to properly add [app links and twitter
cards](https://nextjs.org/docs/app/api-reference/functions/generate-metadata#applinks).
TODO: create ticket
4. Replace i18n's `useTranslation` with next-intl's `useTranslations`
5. Remove hard-coded strings that were present b/c we were still b/w
i18next and next-intl

#### Changes
* [Move process page to
app](32ba4ee)

* [Move research page to
app](5b5ad1a)

* [Move health page to
app](a3e6255)

* [Move feature flag page to
app](395baed)
* [Move search page to app
router](1e261e3)
* [Move newsletter pages to app
router](b509ef8)
* [Move home page to app
router](de1be98)
* [Move home page to app
router](74077ae)
* [Move 404 page to app
router](ccbc956)

### Misc

1. [Delete hello
api](5bad6ea)
     * This was left from the project creation
2. [Add USWDS icon
component](0120c7b)
* as noted in a slack discussion, when trying to access [one of the
icons](https://github.com/trussworks/react-uswds/blob/main/src/components/Icon/Icons.ts)
using `<Icon.Search/>` next errors: `You cannot dot into a client module
from a server component. You can only pass the imported name through`.
I'm not sure why it thinks the Icon component is a client module. [Dan
A.
suggests](vercel/next.js#51593 (comment))
trussworks should re-export as named exports. I tried importing the SVGs
directly from the trussworks library but [svgr requires a custom webpack
config](https://react-svgr.com/docs/next/) which is a road I didn't want
to go down and [react svg](https://www.npmjs.com/package/react-svg)
through an error in the app router 😥 .
* I implemented @sawyerh 's
[suggestion](0120c7b#diff-dadb35bd2f3f61f2c179f033cd0a2874fc343974236f2fb8613664703c751429),
which did not work initially b/c next reported the USWDS icon was
corrupt, which was fixed by adding a `viewBox` to the svg element 😮‍💨 .
* [Remove unused
WtGIContent](75490f7)

### Layout and component updates
* [Move layout and update for app
router](af112fd)

* [Update global components for the app
router](40119e6)

### Remaining next-intl config and removal of 

* [Move i18n strings for app
router](eb3c07c)

* [Adds next-intl config and removes
i18n](c546571)

* [Update tests for app
router](3b9b193)

* [Removes i18next and next-i18n
packages](9d2e08a)

* [Update storybook settings for app
router](39f115d)
## Summary
Fixes #37 


## Changes proposed
- add e2e docs prereq api command
## Summary
Fixes #50

## Changes proposed
- switch to using USWDS `Select` component
## Summary
Fixes #56 

## Changes proposed
- Update the date creation to the parsed year/month/day so it creates
the `Date` object using the local time - this may prevent any type of
rounding
- Add some tests to ensure the right dates are showing up on search
…freshes (#67)

## Summary
Fixes #58

### Time to review: __3 mins__

## Changes proposed
Set the `persistAuthorization` OpenAPI config locally to True

## Context for reviewers
For local development, we frequently need to go to
http://localhost:8080/docs - enter the auth token, and then repeat this
process every time we reopen this page or refresh. Having to either copy
paste or retype in the auth token is tedious. This flag makes it so it
gets preserved in your browsers local storage.

We are only enabling this for the local endpoint at the moment as there
are possibly security implications we would need to consider non-locally
(eg. what if someone is using a public computer).
## Summary
Fixes #51 

## Changes proposed
- Add 300ms debounce on pagination
## Summary
Fixes #64 

## Changes proposed
- update to next 14.2.3 (https://nextjs.org/blog/next-14-2)
## Summary
Fixes #69

### Time to review: __5 mins__

## Changes proposed
Remove the `BASE_RESPONSE_SCHEMA` configuration.

Adjust the healthcheck endpoint to be consistent in defining the schema
/ responses with the other endpoints.

## Context for reviewers
APIFlask allows you to set a `BASE_RESPONSE_SCHEMA` - the idea is that
its the shared schema that every API endpoint will return, and they will
only differ in the `data` object within. This sounds great on paper as
it prevents you from needing to define most of the response schema for
many endpoints, but in practice, its clunky to use. If we want to modify
anything in the response schema outside of the `data` object, it will
affect every single endpoint. This means when we add something like the
pagination info to our search endpoint, a pagination object appears on
the healthcheck response. As we intend to make these docs something the
public will use, this is going to be confusing.

There is also a "bug" (unsure if it is as it was an intended change a
few months ago in APIFlask/apispec) that the error response objects end
up nested within themselves in the examples.

For example, currently the error response for the healthcheck endpoint
(which can literally only return a 5xx error) has an example response
of:
```json
{
  "data": {
    "data": "string",
    "errors": [
      {
        "field": "string",
        "message": "string",
        "type": "string"
      }
    ],
    "message": "string",
    "status_code": 0
  },
  "message": "string",
  "pagination_info": {
    "order_by": "id",
    "page_offset": 1,
    "page_size": 25,
    "sort_direction": "ascending",
    "total_pages": 2,
    "total_records": 42
  },
  "status_code": 0,
  "warnings": [
    {
      "field": "string",
      "message": "string",
      "type": "string"
    }
  ]
}
```
When in reality, the error response it actually returns looks like:
```json
{
  "data": {},
  "errors": [],
  "message": "Service Unavailable",
  "status_code": 503
}
```

This set of changes works around all of these confusing issues and just
requires us to define specific response schemas for each endpoint with
some small set of details. I've kept some base schema classes to derive
from that we can use in most cases.

## Additional information

Before & After Examples in OpenAPI

<table>
<tr><th>Endpoint</th><th>Before</th><th>After</th><th>Actual Response
(before change)</th></tr>
<tr><td>GET /health (200) </td>
<td>

```json
{
  "data": {
    "message": "string"
  },
  "message": "string",
  "pagination_info": {
    "order_by": "id",
    "page_offset": 1,
    "page_size": 25,
    "sort_direction": "ascending",
    "total_pages": 2,
    "total_records": 42
  },
  "status_code": 0,
  "warnings": [
    {
      "field": "string",
      "message": "string",
      "type": "string"
    }
  ]
}
```
</td>
<td>

```json
{
  "data": null,
  "message": "Success",
  "status_code": 200
}
```
</td>
<td>

```json
{
  "data": {},
  "message": "Service healthy",
  "pagination_info": null,
  "status_code": 200,
  "warnings": []
}
```
</td>
</tr>

<tr><td>GET /health (503)</td>
<td>

```json
{
  "data": {
    "data": "string",
    "errors": [
      {
        "field": "string",
        "message": "string",
        "type": "string"
      }
    ],
    "message": "string",
    "status_code": 0
  },
  "message": "string",
  "pagination_info": {
    "order_by": "id",
    "page_offset": 1,
    "page_size": 25,
    "sort_direction": "ascending",
    "total_pages": 2,
    "total_records": 42
  },
  "status_code": 0,
  "warnings": [
    {
      "field": "string",
      "message": "string",
      "type": "string"
    }
  ]
}
```
</td>
<td>

```json
{
  "data": {},
  "errors": [],
  "message": "Error",
  "status_code": 0
}
```
</td>
<td>

```json
{
  "data": {},
  "message": "Service unavailable",
  "pagination_info": null,
  "status_code": 200,
  "warnings": []
}
```
</td>
</tr>

<tr><td>POST /v0.1/opportunities/search (200)</td>
<td>

```json
{
  "data": [
    {.. Excluding for brevity }
  ],
  "message": "string",
  "pagination_info": {
    "order_by": "id",
    "page_offset": 1,
    "page_size": 25,
    "sort_direction": "ascending",
    "total_pages": 2,
    "total_records": 42
  },
  "status_code": 0,
  "warnings": [
    {
      "field": "string",
      "message": "string",
      "type": "string"
    }
  ]
}
```
</td>
<td>

```json
{
  "data": [
    {.. Excluding for brevity }
  ],
  "message": "Success",
  "pagination_info": {
    "order_by": "id",
    "page_offset": 1,
    "page_size": 25,
    "sort_direction": "ascending",
    "total_pages": 2,
    "total_records": 42
  },
  "status_code": 200
}
```
</td>
<td>

```json
{
  "data": [
    {}, {}, {} // excluded for brevity
  ],
  "message": "Success",
  "pagination_info": {
    "order_by": "opportunity_id",
    "page_offset": 1,
    "page_size": 3,
    "sort_direction": "ascending",
    "total_pages": 1010,
    "total_records": 3030
  },
  "status_code": 200,
  "warnings": []
}
```
</td>
</tr>

<tr><td>POST /v0.1/opportunities/search (401 or 422)</td>
<td>

```json
{
  "data": {
    "data": "string",
    "errors": [
      {
        "field": "string",
        "message": "string",
        "type": "string"
      }
    ],
    "message": "string",
    "status_code": 0
  },
  "message": "string",
  "pagination_info": {
    "order_by": "id",
    "page_offset": 1,
    "page_size": 25,
    "sort_direction": "ascending",
    "total_pages": 2,
    "total_records": 42
  },
  "status_code": 0,
  "warnings": [
    {
      "field": "string",
      "message": "string",
      "type": "string"
    }
  ]
}
```
</td>
<td>

```json
{
  "data": {},
  "errors": [],
  "message": "Error",
  "status_code": 0
}
```
</td>
<td>

```json
{
  "data": {},
  "errors": [],
  "message": "The server could not verify that you are authorized to access the URL requested",
  "status_code": 401
}
```
</td>
</tr>

</table>

---------

Co-authored-by: nava-platform-bot <[email protected]>
## Summary
Fixes #45 

### Time to review: __3 mins__

## Changes proposed
* update `config.py` database url
* add function in `cli.py` 
* updated packages in `poetry.lock`

## Context for reviewers
N/A

## Additional information
Row created manually in the database alongside a row created via
`test_connection`
![Screen Shot 2024-06-11 at 1 49 53
PM](https://github.com/navapbc/simpler-grants-gov/assets/37313082/b83afad8-5fe1-404f-adf3-c94945740bbe)
## Summary
Fixes #40 

### Time to review: 5 min

## Changes proposed
- Add `pa11y-ci` to run PR checks
   - Tests for each of the pages we have so far
## Summary
Fixes #88

### Time to review: __5 mins__

## Changes proposed
Remove the version field from the docker-compose files

Adjust the docker commands to use `docker compose` instead of
`docker-compose`

## Context for reviewers
A recent version of docker removed the need for specifying a version in
the compose files - so the field is now obsolete and just gives a
warning whenever you run a command:
https://docs.docker.com/compose/compose-file/04-version-and-name/

The change in the command we run is actually from 2021 and makes sure we
use docker compose v2. Depending on how you've setup your local instance
of docker, `docker-compose` may have been aliased to `docker compose`
anyways (I actually think if it wasn't, certain things break anyways).
This is just using the proper format for certain.

## Additional information
I went through running several docker commands and noticed no
difference, as this change shouldn't meaningfully change anything, that
is to be expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants